fix(sc): keep the watchdog armed while the train pump drains - #3783
Open
tianyi-zhang-02 wants to merge 4 commits into
Open
fix(sc): keep the watchdog armed while the train pump drains#3783tianyi-zhang-02 wants to merge 4 commits into
tianyi-zhang-02 wants to merge 4 commits into
Conversation
tianyi-zhang-02
force-pushed
the
fix/sc-run-supervision
branch
2 times, most recently
from
August 27, 2026 02:14
f3d0b8e to
a7ea16c
Compare
tianyi-zhang-02
added a commit
to tianyi-zhang-02/RL
that referenced
this pull request
Aug 27, 2026
…ge reads Nine controller stubs in this file predate that attribute, so they pass here and fail once merged with current main -- NVIDIA-NeMo#3768 made _advantage_stage read it unconditionally. Same shape as the _rollout_manager stub gap in NVIDIA-NeMo#3783. Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
tianyi-zhang-02
force-pushed
the
fix/sc-run-supervision
branch
from
August 28, 2026 15:29
a7ea16c to
caa36ad
Compare
run() waits once with FIRST_COMPLETED, handles whichever task finished, then falls through to a bare `await train_task`. From that point the watchdog and the fleet probe are still running but nothing awaits them again, and the `finally` retrieves their exceptions with gather(return_exceptions=True) and discards them -- so not even asyncio's "Task exception was never retrieved" warning fires. The window this opens is the normal path, not an edge case. The rollout pump finishing first is end-of-data, as run()'s own comment says: "A normally exhausted rollout pump leaves the train pump to drain committed groups." For the whole of that drain, a RolloutStall under stall_action: abort, a GenerationFleetExhausted, or an env-health abort lands on an unobserved task while run() parks on the wedged train pump. The job then holds its GPUs until the scheduler's wall clock kills it, with nothing in the driver log -- which is what WatchdogConfig, documented as "Last-resort detection for stalls that no other layer catches", exists to prevent. Loop the wait instead, so every task stays supervised until the train pump exits. The priority order (probe, watchdog, rollout, train) is unchanged, and so is the behaviour when the train pump finishes first. Three tests. The drain-phase one fails on main with a TimeoutError rather than the RolloutStall it asserts; the other two pin the paths that already worked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
The three tests here all take the watchdog branch. Two others were unreached:
- rollout-pump failure. Every test has the rollout pump either exhaust
cleanly or never finish, so the branch whose comment promises immediate
propagation was never exercised. It is the branch that matters most under
the old single wait: a failed rollout task sits in pending, unawaited,
while run() parks on the train pump.
- the fleet probe. _bare_actor sets _gen_fleet = None, so run() creates no
probe task at all and the "probe_task in done" branch was dead in every
test.
Both use a distinct exception type so a test cannot pass by catching the
wrong task's failure.
Mutation-tested: collapsing the loop, and skipping either await, each turn
one of these red.
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
…pumps The branch predated that call, so the test passed here and failed once merged with current main -- mergeable, but red afterwards. Only showed up when I merged all twenty of my open PRs together and bisected back. Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com> Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
tianyi-zhang-02
force-pushed
the
fix/sc-run-supervision
branch
from
September 9, 2026 17:30
7d740e9 to
c037cb7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Keeps the SingleController train pump, rollout pump, stall watchdog, generation-fleet probe, and rollout-checkpoint pump supervised until training exits.
Previously
run()waited only once withFIRST_COMPLETED. A normally exhausted rollout pump could leave the train pump draining while watchdog and fleet-probe failures were no longer observed. The supervision loop now keeps every pending task armed, preserves the existing failure priority, and treats the checkpoint pump's requested shutdown separately from an unexpected exit.The tests cover watchdog failure before and after rollout exhaustion, clean completion, rollout failure during a wedged train pump, fleet-probe failure during the drain, and both expected and unexpected checkpoint-pump completion.
Validation
Current head
c037cb7b2d5f9e9b1a9a490e6038c452a0c855d0, rebased on upstreammainatf0f85513af2d232c435100b0656b31db25dfbcd5.